-
Notifications
You must be signed in to change notification settings - Fork 120
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
move cli to extra file, macOS tests add documentation #501
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Summary
This PR makes significant changes to documentation and CLI functionality in the Infinity project. Here's a concise summary of the key changes:
- Introduces new CLI architecture in
cli.py
with v2 command supporting multiple model loading and enhanced configuration options - Moves CLI-related code from
infinity_server.py
to dedicatedcli.py
module for better separation of concerns - Adds documentation for batch handling and caching mechanisms, though some docstrings remain incomplete
- Problematically removes entire contents of
docs/docs/index.md
without replacement, losing critical user documentation - Adds test coverage for new CLI functionality with parameterized tests for both v1 and v2 commands
Key concerns:
- Complete removal of index.md documentation needs to be addressed
- Incomplete docstrings in caching layer need completion
- No clear documentation for nomic-ai/nomic-embed-text-v1.5 support mentioned in issue Support for nomic-ai/nomic-embed-text-v1.5 #123
- Caching implementation may have thread safety issues that should be reviewed
- CLI tests are skipped on Windows platform
8 file(s) reviewed, 7 comment(s)
Edit PR Review Bot Settings | Greptile
```bash | ||
cd libs/infinity_emb | ||
make precommit | ||
``` | ||
|
||
## CLA | ||
Infinity is developed as open source project. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: consider rephrasing to 'Infinity is an open source project' for better clarity
"""Infinity API ♾️ cli v2. MIT License. Copyright (c) 2023-now Michael Feil \n | ||
\n | ||
Multiple Model CLI Playbook: \n | ||
- 1. cli options can be overloaded i.e. `v2 --model-id model/id1 --model-id/id2 --batch-size 8 --batch-size 4` \n |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
syntax: incorrect example in docs: --model-id/id2
should be --model-id model/id2
trust_remote_code=trust_remote_code, | ||
engine=engine, | ||
model_warmup=model_warmup, | ||
vector_disk_cache_path=vector_disk_cache, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: incorrect parameter name: vector_disk_cache_path should be vector_disk_cache to match the input parameter name
"""wrapper around DiskCache. The Diskcache in infinity `races` against the model inference. | ||
|
||
The concept is that with the code be | ||
""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
syntax: Docstring is incomplete and unclear. The sentence 'The concept is that with the code be' is unfinished. Please complete the explanation of how the cache races against model inference.
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #501 +/- ##
==========================================
+ Coverage 79.98% 80.06% +0.08%
==========================================
Files 42 43 +1
Lines 3467 3471 +4
==========================================
+ Hits 2773 2779 +6
+ Misses 694 692 -2 ☔ View full report in Codecov by Sentry. |
Related Issue
Checklist
Additional Notes
Add any other context about the PR here.